[clang] Fix -Wuninitialized for values passed by const pointers #147221

igorkudrin · 2025-07-07T00:17:10Z

This enables producing a "variable is uninitialized" warning when a value is passed to a pointer-to-const argument:

void foo(const int *);
void test() {
  int *v;
  foo(v);
}

Fixes #37460

This enables producing a "variable is uninitialized" warning when a value is passed to a pointer-to-const argument: ``` void foo(const int *); void test() { int *v; foo(v); } ``` Fixes llvm#37460

llvmbot · 2025-07-07T00:17:30Z

@llvm/pr-subscribers-libcxx
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Igor Kudrin (igorkudrin)

Changes

This enables producing a "variable is uninitialized" warning when a value is passed to a pointer-to-const argument:

void foo(const int *);
void test() {
  int *v;
  foo(v);
}

Fixes #37460

Full diff: https://github.com/llvm/llvm-project/pull/147221.diff

2 Files Affected:

(modified) clang/lib/Analysis/UninitializedValues.cpp (+5-8)
(modified) clang/test/SemaCXX/uninitialized.cpp (+6-2)

diff --git a/clang/lib/Analysis/UninitializedValues.cpp b/clang/lib/Analysis/UninitializedValues.cpp
index b2a68b6c39a7e..540838f89f20c 100644
--- a/clang/lib/Analysis/UninitializedValues.cpp
+++ b/clang/lib/Analysis/UninitializedValues.cpp
@@ -438,13 +438,10 @@ void ClassifyRefs::VisitCallExpr(CallExpr *CE) {
     return;
   }
   bool isTrivialBody = hasTrivialBody(CE);
-  // If a value is passed by const pointer to a function,
-  // we should not assume that it is initialized by the call, and we
-  // conservatively do not assume that it is used.
-  // If a value is passed by const reference to a function,
-  // it should already be initialized.
-  for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end();
-       I != E; ++I) {
+  // A value passed by const pointer or reference to a function should already
+  // be initialized.
+  for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end(); I != E;
+       ++I) {
     if ((*I)->isGLValue()) {
       if ((*I)->getType().isConstQualified())
         classify((*I), isTrivialBody ? Ignore : ConstRefUse);
@@ -453,7 +450,7 @@ void ClassifyRefs::VisitCallExpr(CallExpr *CE) {
       const auto *UO = dyn_cast<UnaryOperator>(Ex);
       if (UO && UO->getOpcode() == UO_AddrOf)
         Ex = UO->getSubExpr();
-      classify(Ex, Ignore);
+      classify(Ex, Use);
     }
   }
 }
diff --git a/clang/test/SemaCXX/uninitialized.cpp b/clang/test/SemaCXX/uninitialized.cpp
index c7b987e2172e6..4a944ba830bc3 100644
--- a/clang/test/SemaCXX/uninitialized.cpp
+++ b/clang/test/SemaCXX/uninitialized.cpp
@@ -162,12 +162,16 @@ void test_const_ptr() {
   int a;
   int b;  // expected-note {{initialize the variable 'b' to silence this warning}}
   foo(&a);
-  bar(&b);
-  b = a + b; // expected-warning {{variable 'b' is uninitialized when used here}}
+  bar(&b); // expected-warning {{variable 'b' is uninitialized when used here}}
+  b = a + b;
   int *ptr;  //expected-note {{initialize the variable 'ptr' to silence this warning}}
   const int *ptr2;
   foo(ptr); // expected-warning {{variable 'ptr' is uninitialized when used here}}
   foobar(&ptr2);
+  int *ptr3; // expected-note {{initialize the variable 'ptr3' to silence this warning}}
+  const int *ptr4; // expected-note {{initialize the variable 'ptr4' to silence this warning}}
+  bar(ptr3); // expected-warning {{variable 'ptr3' is uninitialized when used here}}
+  bar(ptr4); // expected-warning {{variable 'ptr4' is uninitialized when used here}}
 }
 }

Xazax-hun

LGTM! I have a nit inline for your consideration.

Xazax-hun · 2025-07-07T10:47:25Z

clang/lib/Analysis/UninitializedValues.cpp

-       I != E; ++I) {
+  // A value passed by const pointer or reference to a function should already
+  // be initialized.
+  for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end(); I != E;


Could this use CE->arguments() in a range based for loop instead?

I'd prefer not to do that to keep the fix as atomic as possible and to avoid concealing the actual behavior change among unrelated NFC changes.

Xazax-hun · 2025-07-07T10:50:48Z

Actually, before merging, did you manage to run this on any non-trivial projects to see if there are any false positives? I think this change is valuable and important but these sorts of changes can expose some false positives. If that is the case (or there are some noisy patterns), we might want to do the stricter version of the analysis under a different flag.

ldionne

libc++ changes LGTM.

zygoloid · 2025-07-07T20:53:09Z

clang/lib/Analysis/UninitializedValues.cpp

      if (UO && UO->getOpcode() == UO_AddrOf)
        Ex = UO->getSubExpr();
-      classify(Ex, Ignore);
+      classify(Ex, Use);


I believe the intent of this code is that if the address of a local variable is passed to a const pointer, that local variable is neither considered used nor initialized. When a local variable is passed directly to a call, no special handling should be required, because an lvalue-to-rvalue conversion will be performed, which will be classified as a use. So I think the correct fix is instead:

Suggested change

if (UO && UO->getOpcode() == UO_AddrOf)

Ex = UO->getSubExpr();

classify(Ex, Ignore);

classify(Ex, Use);

if (UO && UO->getOpcode() == UO_AddrOf) {

classify(UO->getSubExpr(), Ignore);

}

That said, rather than treating this case as Ignore, it seems best to treat it instead as ConstRefUse, since passing a local variable by const reference and passing its address by const pointer are analogous and should be treated the same way.

Your suggestion does not cover the case:

void foo(const int *); void test() { int v; foo(&v); }

Note that gcc catches it: https://godbolt.org/z/TdKG35shz

That said, rather than treating this case as Ignore, it seems best to treat it instead as ConstRefUse, since passing a local variable by const reference and passing its address by const pointer are analogous and should be treated the same way.

TBH, I don't think we need a separate diagnostic for ConstRefUse while it can be reported as any other use of an uninitialized value. But changing that is not the purpose of this patch.

Your suggestion does not cover the case:

void foo(const int *); void test() { int v; foo(&v); }

Correct. We intentionally don't warn on that -- a "const reference use" is neither treated as initializing nor as using the local variable.

Correct. We intentionally don't warn on that -- a "const reference use" is neither treated as initializing nor as using the local variable.

What is the basis of this intention?

Warning on such uses was found to lead to false positives. Such as all the cases you found in libc++'s test suite :)

The point is, for foo(&v), where foo takes a const pointer, we simply don't know what foo does to v. It might read from it, or it might just store a pointer to it somewhere. And -Wuninitialized / -Wsometimes-uninitialized are supposed to have zero false positives. What we do know is that foo does not initialize v, which is why we were marking it as ignore here (because otherwise we would treat a pointer to v as escaping, which conservatively marks it initialized).

With my suggested approach plus changing the kind from Ignore to ConstRefUse, we'd start warning under -Wuninitialized-const-reference (which is a subgroup of -Wuninitialized but should be a subgroup of -Wconditional-uninitialized, but that's a separate bug). That'd be consistent and probably reasonable, other than the warning message and flag being a bit inaccurate for this case, and wouldn't introduce false positives into the other uninitialized warning flags (that are supposed to not have false positives).

Thank you for the explanation. These two cases, passing an uninitialized value and passing the address of such a value, are indeed different, because the address is defined. I guess that implementing your second suggestion, i.e. to use ConstRefUse instead of Ignore, requires a lot more changes; at least, the warning message needs to be updated to reflect the actual case. I'll prepare a separate PR for this.

igorkudrin · 2025-07-07T21:11:13Z

Actually, before merging, did you manage to run this on any non-trivial projects to see if there are any false positives? I think this change is valuable and important but these sorts of changes can expose some false positives. If that is the case (or there are some noisy patterns), we might want to do the stricter version of the analysis under a different flag.

I've built the whole LLVM project with -Werror=uninitialized. No cases were detected, neither true nor false. Do you think this is enough?

Co-authored-by: Richard Smith <richard@metafoo.co.uk>

[clang] Fix -Wuninitialized for values passed by const pointers

f1e26fe

This enables producing a "variable is uninitialized" warning when a value is passed to a pointer-to-const argument: ``` void foo(const int *); void test() { int *v; foo(v); } ``` Fixes llvm#37460

igorkudrin requested review from rnk, zygoloid, MaskRay, r4nt, AaronBallman, aeubanks, ZequanWu and dwblaikie July 7, 2025 00:17

igorkudrin added clang Clang issues not falling into any other category clang:analysis labels Jul 7, 2025

fixup! libcxx tests

d63b5dc

igorkudrin requested a review from a team as a code owner July 7, 2025 01:41

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 7, 2025

igorkudrin requested a review from philnik777 July 7, 2025 01:42

Xazax-hun approved these changes Jul 7, 2025

View reviewed changes

ldionne approved these changes Jul 7, 2025

View reviewed changes

zygoloid requested changes Jul 7, 2025

View reviewed changes

aeubanks removed their request for review July 7, 2025 22:00

fixup! Do not detect passing a value by a constant pointer

4ff02a7

Co-authored-by: Richard Smith <richard@metafoo.co.uk>

[clang] Fix -Wuninitialized for values passed by const pointers #147221

Are you sure you want to change the base?

[clang] Fix -Wuninitialized for values passed by const pointers #147221

Uh oh!

Conversation

igorkudrin commented Jul 7, 2025

Uh oh!

llvmbot commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Xazax-hun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xazax-hun commented Jul 7, 2025

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zygoloid Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zygoloid Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorkudrin commented Jul 7, 2025

Uh oh!

Uh oh!

llvmbot commented Jul 7, 2025 •

edited

Loading

zygoloid Jul 7, 2025 •

edited

Loading

zygoloid Jul 7, 2025 •

edited

Loading